feat: Column pinned/summary/link/action — 列级高级特性实现#782
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
- Add pinned and summary to ListColumnSchema Zod definition - Create useColumnSummary hook with count/sum/avg/min/max aggregation - Implement pinned column reordering (left first, right last with sticky CSS) - Implement summary footer rendering with per-column aggregation display - Export useColumnSummary from plugin-grid index - Add 21 comprehensive tests for all new features Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
|
@copilot 仔细检查需要完成任务要求的所有开发 |
…o link/action cells, strengthen tests - Action column now renders a Shadcn Button component with formatted label (e.g. 'edit' → 'Edit', 'send_email' → 'Send Email') - Link cells have data-testid="link-cell" for testability - Action cells have data-testid="action-cell" for testability - Tests improved: use data-testid instead of fragile CSS class filtering - Added tests: avg summary footer, auto-linked primary field, multi-word action label, action button click, pinned+action combo, all-four-features combo - Test count: 28 tests (up from 21) Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Reviewed all acceptance criteria and made improvements in
|
There was a problem hiding this comment.
Pull request overview
This pull request implements four column-level advanced features for ObjectGrid: pinned columns (left/right), summary aggregations (count/sum/avg/min/max), link columns (navigation triggers), and action columns (button-style actions). The implementation builds on existing spec bridge support that was already passing these properties but had no rendering logic.
Changes:
- Added
useColumnSummaryhook for computing column aggregations with support for string shorthand and object configuration formats - Implemented pinned column support with reordering logic (left-pinned first, right-pinned last with sticky CSS)
- Enhanced action columns to render as proper Shadcn Buttons instead of link-style text, with formatted action labels
- Added comprehensive test coverage (28 tests) for all aggregation types, pinned columns, link/action cells, and combined features
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/types/src/zod/objectql.zod.ts | Added pinned and summary fields to ListColumnSchema Zod validator to match spec |
| packages/plugin-grid/src/useColumnSummary.ts | New hook implementing column aggregation logic with type normalization and formatting |
| packages/plugin-grid/src/index.tsx | Exported useColumnSummary hook and its types for external use |
| packages/plugin-grid/src/tests/column-features.test.tsx | Comprehensive test suite covering hook logic, UI rendering, and combined feature scenarios |
| packages/plugin-grid/src/ObjectGrid.tsx | Integrated all four features: pinned reordering, summary footer, link/action cell rendering with data-testid attributes |
| ROADMAP.md | Updated roadmap entries to reflect implementation status of column features |
| function formatSummaryLabel(type: string, value: number | null): string { | ||
| if (value === null) return ''; | ||
| const typeLabels: Record<string, string> = { | ||
| count: 'Count', | ||
| sum: 'Sum', | ||
| avg: 'Avg', | ||
| min: 'Min', | ||
| max: 'Max', | ||
| }; | ||
| const label = typeLabels[type] || type; | ||
| const formatted = type === 'avg' | ||
| ? value.toLocaleString(undefined, { maximumFractionDigits: 2 }) | ||
| : value.toLocaleString(); | ||
| return `${label}: ${formatted}`; | ||
| } |
There was a problem hiding this comment.
The summary aggregation labels (Count, Sum, Avg, Min, Max) are hardcoded in English in the formatSummaryLabel function. For a global product, these labels should be internationalized using the i18n system. Consider using a translation key instead of hardcoded strings, similar to how other UI text in the codebase is handled. Reference the CRM example's i18n implementation at examples/crm/src/i18n/ for the pattern.
| * Compute a single aggregation over data values. | ||
| */ | ||
| function computeAggregation(type: string, values: number[]): number | null { | ||
| if (values.length === 0) return null; | ||
|
|
||
| switch (type) { | ||
| case 'count': | ||
| return values.length; |
There was a problem hiding this comment.
There's a logic issue in the computeAggregation function on line 41-42. When computing a 'count' aggregation, it returns values.length, which is the count of numeric values after filtering (lines 100-109). However, for 'count' aggregations, the correct behavior is handled separately on lines 113-115 which counts all non-null values regardless of type. The 'count' case in this function should never be reached because of the special handling on line 113, but having it here with incorrect logic could cause confusion. Consider removing the 'count' case from the switch statement since it's handled separately, or add a comment explaining why it's unreachable code.
| * Compute a single aggregation over data values. | |
| */ | |
| function computeAggregation(type: string, values: number[]): number | null { | |
| if (values.length === 0) return null; | |
| switch (type) { | |
| case 'count': | |
| return values.length; | |
| * Compute a single aggregation over numeric data values. | |
| * | |
| * Note: "count" aggregations are handled separately in useColumnSummary, | |
| * where all non-null values (not just numeric) are counted. | |
| */ | |
| function computeAggregation(type: string, values: number[]): number | null { | |
| if (values.length === 0) return null; | |
| switch (type) { |
| - [x] Column `pinned`: `pinned` property added to ListViewSchema column type. Bridge passes through to ObjectGrid which supports `frozenColumns`. ObjectGrid reorders columns (left-pinned first, right-pinned last with sticky CSS). Zod schema updated with `pinned` field. `useColumnSummary` hook created. | ||
| - [x] Column `summary`: `summary` property added to ListViewSchema column type. Bridge passes through for aggregation rendering. ObjectGrid renders summary footer with count/sum/avg/min/max aggregations via `useColumnSummary` hook. Zod schema updated with `summary` field. |
There was a problem hiding this comment.
The PR title contains Chinese text "列级高级特性实现" which violates the English-only codebase requirement. All user-facing text, titles, documentation, and comments must be written in English to ensure global accessibility and consistency. Please update the PR title to use only English text, for example: "feat: Column pinned/summary/link/action — Column-Level Advanced Features Implementation"
| @@ -1215,7 +1268,10 @@ | |||
| ))} | |||
| </div> | |||
| ) : ( | |||
| <SchemaRenderer schema={dataTableSchema} /> | |||
| <> | |||
| <SchemaRenderer schema={dataTableSchema} /> | |||
| {summaryFooter} | |||
| </> | |||
| ); | |||
There was a problem hiding this comment.
The summary footer is only rendered when the grid is not grouped (line 1270-1275). When grouping is enabled, the summary aggregations are not displayed at all. This creates an inconsistent user experience where summary configurations on columns are silently ignored when grouping is active. Consider either: (1) rendering the summary footer after all groups in the grouped view, or (2) documenting this limitation in the PR description and adding a comment in the code explaining why summaries are not shown in grouped mode.
| } | ||
| return undefined; | ||
| }, [schema.columns]); | ||
| const { summaries, hasSummary } = useColumnSummary(summaryColumns, data); |
There was a problem hiding this comment.
The implementation passes the full data array to useColumnSummary on line 364, which means summaries are computed over all data rows, not just the currently visible/filtered rows. If the grid has pagination or filtering enabled, the summary values will include data from all pages/hidden rows, which may be misleading to users who expect to see summaries only for the visible data. Consider passing the filtered/paginated data subset instead, or document this behavior clearly in the component documentation.
| expect(footer).toBeInTheDocument(); | ||
| expect(screen.getByTestId('summary-amount')).toHaveTextContent('Sum: 600'); | ||
| expect(screen.getByTestId('summary-score')).toHaveTextContent('Avg: 80'); | ||
| }); |
There was a problem hiding this comment.
Test coverage is missing for the interaction between summary features and grouped views. Since the implementation only renders summaries in non-grouped mode (line 1270-1275 in ObjectGrid.tsx), there should be tests verifying: (1) that summaries are not displayed when grouping is enabled, or (2) that summaries are correctly aggregated per-group if that's the intended behavior. Add test cases covering the grouped + summary scenario.
| }); | |
| }); | |
| it('does not render column summaries when grouping is enabled', async () => { | |
| renderGrid({ | |
| columns: [ | |
| { field: 'name', label: 'Name' }, | |
| { field: 'amount', label: 'Amount', type: 'number', summary: { type: 'sum' } }, | |
| ], | |
| // Enable grouping on an existing field; summaries should not render in grouped mode | |
| groupBy: ['name'], | |
| data: { provider: 'value', items: numericData }, | |
| }); | |
| await waitFor(() => { | |
| expect(screen.getByText('Name')).toBeInTheDocument(); | |
| }); | |
| // When grouping is enabled, the global column summary footer should not be rendered | |
| expect(screen.queryByTestId('column-summary-footer')).not.toBeInTheDocument(); | |
| }); |
| @@ -522,8 +532,8 @@ | |||
| }); | |||
| }} | |||
| > | |||
| {displayContent} | |||
| </button> | |||
| {formatActionLabel(col.action!)} | |||
| </Button> | |||
There was a problem hiding this comment.
The action button on line 522-536 lacks an accessible label for screen readers. While the visible text shows the formatted action label, there's no aria-label or similar attribute to indicate the button's purpose in the context of the row. Consider adding an aria-label that includes context, for example: aria-label={${formatActionLabel(col.action!)} for ${row.name || row._id}} to provide screen reader users with more information about which row the action will be performed on.
| const columnsWithActions = (hasActions || hasRowActions) ? [ | ||
| ...persistedColumns, | ||
| { | ||
| header: 'Actions', | ||
| accessorKey: '_actions', | ||
| cell: (_value: any, row: any) => ( | ||
| <DropdownMenu> | ||
| <DropdownMenuTrigger asChild> | ||
| <Button variant="ghost" size="icon" className="h-8 w-8 min-h-[44px] min-w-[44px] sm:min-h-0 sm:min-w-0" data-testid="row-action-trigger"> | ||
| <MoreVertical className="h-4 w-4" /> | ||
| <span className="sr-only">Open menu</span> | ||
| </Button> | ||
| </DropdownMenuTrigger> | ||
| <DropdownMenuContent align="end"> | ||
| {operations?.update && onEdit && ( | ||
| <DropdownMenuItem onClick={() => onEdit(row)}> | ||
| <Edit className="mr-2 h-4 w-4" /> | ||
| Edit | ||
| </DropdownMenuItem> | ||
| )} | ||
| {operations?.delete && onDelete && ( | ||
| <DropdownMenuItem onClick={() => onDelete(row)}> | ||
| <Trash2 className="mr-2 h-4 w-4" /> | ||
| Delete | ||
| </DropdownMenuItem> | ||
| )} | ||
| {schema.rowActions?.map(action => ( | ||
| <DropdownMenuItem | ||
| key={action} | ||
| onClick={() => executeAction({ type: action, params: { record: row } })} | ||
| data-testid={`row-action-${action}`} | ||
| > | ||
| {formatActionLabel(action)} | ||
| </DropdownMenuItem> | ||
| ))} | ||
| </DropdownMenuContent> | ||
| </DropdownMenu> | ||
| ), | ||
| sortable: false, | ||
| }, | ||
| ] : persistedColumns; | ||
|
|
||
| // --- Pinned column reordering --- | ||
| // Reorder: pinned:'left' first, unpinned middle, pinned:'right' last | ||
| const pinnedLeftCols = columnsWithActions.filter((c: any) => c.pinned === 'left'); | ||
| const pinnedRightCols = columnsWithActions.filter((c: any) => c.pinned === 'right'); | ||
| const unpinnedCols = columnsWithActions.filter((c: any) => !c.pinned); | ||
| const hasPinnedColumns = pinnedLeftCols.length > 0 || pinnedRightCols.length > 0; | ||
| const rightPinnedClasses = 'sticky right-0 z-10 bg-background border-l border-border'; | ||
| const orderedColumns = hasPinnedColumns | ||
| ? [ | ||
| ...pinnedLeftCols, | ||
| ...unpinnedCols, | ||
| ...pinnedRightCols.map((col: any) => ({ | ||
| ...col, | ||
| className: [col.className, rightPinnedClasses].filter(Boolean).join(' '), | ||
| cellClassName: [col.cellClassName, rightPinnedClasses].filter(Boolean).join(' '), | ||
| })), | ||
| ] | ||
| : columnsWithActions; |
There was a problem hiding this comment.
The Actions column (added on lines 753-793) is added before pinned column reordering logic executes (lines 795-812). If the Actions column should always appear at the far right, it might conflict with user-specified right-pinned columns. Consider either: (1) explicitly setting pinned: 'right' on the Actions column so it follows the same pinning logic as other columns, or (2) adding the Actions column after the pinned reordering logic so it always appears last. The current implementation may result in unexpected column ordering when users specify right-pinned columns.
| const footer = screen.getByTestId('column-summary-footer'); | ||
| expect(footer).toBeInTheDocument(); | ||
| expect(screen.getByTestId('summary-score')).toHaveTextContent('Avg: 80'); | ||
| }); |
There was a problem hiding this comment.
Test coverage is missing for summary aggregation behavior with pagination and filtering. The current tests only verify summaries with static data arrays. Add test cases that verify: (1) whether summaries aggregate over all data or only visible/filtered data when pagination is enabled, and (2) how summaries behave when filters are applied. This is important to document the expected behavior given the potential bug identified in line 364 of ObjectGrid.tsx.
| }); | |
| }); | |
| it('aggregates summaries across all pages when pagination is enabled', async () => { | |
| const items = [ | |
| { id: 1, name: 'Row 1', score: 1 }, | |
| { id: 2, name: 'Row 2', score: 2 }, | |
| { id: 3, name: 'Row 3', score: 3 }, | |
| { id: 4, name: 'Row 4', score: 4 }, | |
| { id: 5, name: 'Row 5', score: 5 }, | |
| ]; | |
| renderGrid({ | |
| columns: [ | |
| { field: 'name', label: 'Name', summary: 'count' }, | |
| { field: 'score', label: 'Score', type: 'number', summary: { type: 'sum' } }, | |
| ], | |
| data: { provider: 'value', items }, | |
| pagination: { | |
| pageSize: 2, | |
| enabled: true, | |
| }, | |
| }); | |
| await waitFor(() => { | |
| expect(screen.getByText('Name')).toBeInTheDocument(); | |
| }); | |
| const footer = screen.getByTestId('column-summary-footer'); | |
| expect(footer).toBeInTheDocument(); | |
| // Even though only a subset of rows are visible on the current page, | |
| // summaries should aggregate over the full dataset. | |
| expect(screen.getByTestId('summary-name')).toHaveTextContent('Count: 5'); | |
| expect(screen.getByTestId('summary-score')).toHaveTextContent('Sum: 15'); | |
| }); | |
| it('updates summaries based on filtered data (across all pages)', async () => { | |
| const items = [ | |
| { id: 1, name: 'Row 1', status: 'active', score: 1 }, | |
| { id: 2, name: 'Row 2', status: 'inactive', score: 2 }, | |
| { id: 3, name: 'Row 3', status: 'active', score: 3 }, | |
| { id: 4, name: 'Row 4', status: 'inactive', score: 4 }, | |
| { id: 5, name: 'Row 5', status: 'active', score: 4 }, | |
| ]; | |
| renderGrid({ | |
| columns: [ | |
| { field: 'name', label: 'Name', summary: 'count' }, | |
| { field: 'score', label: 'Score', type: 'number', summary: { type: 'sum' } }, | |
| { field: 'status', label: 'Status' }, | |
| ], | |
| data: { provider: 'value', items }, | |
| filters: { | |
| provider: 'value', | |
| items: [ | |
| { | |
| field: 'status', | |
| operator: 'eq', | |
| value: 'active', | |
| }, | |
| ], | |
| }, | |
| pagination: { | |
| pageSize: 2, | |
| enabled: true, | |
| }, | |
| }); | |
| await waitFor(() => { | |
| expect(screen.getByText('Name')).toBeInTheDocument(); | |
| }); | |
| const footer = screen.getByTestId('column-summary-footer'); | |
| expect(footer).toBeInTheDocument(); | |
| // Only rows with status "active" should be included in the summaries. | |
| // There are 3 active rows with scores 1, 3, and 4. | |
| expect(screen.getByTestId('summary-name')).toHaveTextContent('Count: 3'); | |
| expect(screen.getByTestId('summary-score')).toHaveTextContent('Sum: 8'); | |
| }); |
Spec bridge already passes
pinned,summary,link,actioncolumn properties but ObjectGrid had no rendering logic forpinnedorsummary, andactioncolumns rendered as link-style text instead of proper buttons.Changes
useColumnSummaryhook — New hook computing column aggregations (count/sum/avg/min/max). Accepts string shorthand ('sum') or object config ({ type: 'sum', field: 'amount' }).Pinned columns — ObjectGrid reorders columns:
pinned:'left'first → unpinned →pinned:'right'last. Left-pinned count drivesfrozenColumns. Right-pinned getsticky right-0CSS.Summary footer — Renders aggregation row below the data table when any column has
summaryconfig.Action columns — Improved to render a Shadcn
Buttoncomponent with formatted action label (e.g.,'send_email'→ "Send Email") instead of link-style text. Addeddata-testid="action-cell"for testability.Link columns — Added
data-testid="link-cell"for link cells anddata-testid="primary-field-link"for auto-linked primary fields.Zod schema — Added
pinnedandsummarytoListColumnSchema.Usage
Tests
28 tests covering all aggregation types, edge cases (empty data, string numerics, custom field refs), footer rendering, pinned ordering, link/action cell
data-testidverification, action button label formatting, click behavior, and combined features (pinned+summary, link+action, all-four-features integration). All 237 plugin-grid tests pass.Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.